-
Notifications
You must be signed in to change notification settings - Fork 5.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
introduce publish (alpha) command #10949
Conversation
e9622c9
to
ee6e7b9
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #10949 +/- ##
==========================================
- Coverage 58.30% 57.73% -0.57%
==========================================
Files 124 127 +3
Lines 10912 11037 +125
==========================================
+ Hits 6362 6372 +10
- Misses 3922 4034 +112
- Partials 628 631 +3
☔ View full report in Codecov by Sentry. |
1b302de
to
c74d06c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic LGTM, just a couple higher-level things
pkg/remote/oci.go
Outdated
} | ||
|
||
func (g ociRemoteLoader) Accept(path string) bool { | ||
return strings.HasPrefix(path, "docker:") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe elsewhere (e.g. build) we've used docker-image://
as the format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this isn't technically an image, but a new artifact type, at which point I wonder if oci://
would be better?
(I know flux2 uses that format for example: https://fluxcd.io/flux/cheatsheets/oci-artifacts/#consuming-artifacts)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oci
would be a better terminology from a pure implementation point of view, but I don't expect more than 1% Docker users know about Open Container Initiative, and just have a basic understanding of Docker images
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely agree that most people don't know what OCI is 😛
I would still say that:
- We should at least use a URL scheme, i.e.
foo://bar
instead offoo:bar
, being able to do a cheap check for://
to distinguish between local & remote (without awareness of loader) is a helpful property docker
isn't really descriptive and could mean many different things/is un-Googleable, but "Docker Compose OCI include" will [hopefully] get results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
helm also uses oci://
for modern helm push/pull
if err != nil { | ||
return "", err | ||
} | ||
return project.Name, nil | ||
} | ||
|
||
func (o *ProjectOptions) ToProject(services []string, po ...cli.ProjectOptionsFn) (*types.Project, error) { | ||
func (o *ProjectOptions) ToProject(dockerCli command.Cli, services []string, po ...cli.ProjectOptionsFn) (*types.Project, error) { | ||
if !o.Offline { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this was a very elegant solution! ✨
Oh no, I caused a bunch of conflicts with the build changes...sorry 😭 |
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me
00dad5e
to
08a7873
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, few last things but mostly the distribution/distribution/v3
dep that snuck back 😅
go.mod
Outdated
@@ -50,6 +50,8 @@ require ( | |||
gotest.tools/v3 v3.5.0 | |||
) | |||
|
|||
require github.com/distribution/distribution/v3 v3.0.0-20230214150026-36d8c594d7aa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #10954 - will need to swap things to github.com/distribution/reference
to keep this from reappearing.
pkg/api/api.go
Outdated
@@ -74,6 +74,8 @@ type Service interface { | |||
Events(ctx context.Context, projectName string, options EventsOptions) error | |||
// Port executes the equivalent to a `compose port` | |||
Port(ctx context.Context, projectName string, service string, port uint16, options PortOptions) (string, int, error) | |||
// Publish executes the equivalent to a `compose publish` | |||
Publish(ctx context.Context, project *types.Project, repository string) error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: we often end up refactoring the API methods to add an options object later...maybe worth having PublishOptions
with a Repository string
field upfront
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. I'm guessing this will need Quiet bool
at some point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repository being mandatory parameter should not be part of the Options
struct imho
offline bool | ||
} | ||
|
||
const prefix = "oci:" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be oci://
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not an URL, just a prefix we decide on our own. There's no canonical URL style for OCI artifacts, as this ends being transported over http
04b3fe6
to
998a7ea
Compare
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
This feature seems very exciting! I found the documentation on this feature (https://docs.docker.com/engine/reference/commandline/compose_alpha_publish/) but are there more resources to learn how to use it and play around with it? My questions:
|
then you can use this published model from another compose file using
swarm doesn't use the compose specification nor compose-go to parse compose files. |
This introduce new
docker compose (alpha) publish REPOSITORY
command to publish a compose.yaml file as an OCI artifact, and support fordocker:<REPOSITORY>
remote resource to be used withinclude
illustration example:
note: most of the changes in this PR is about passing
dockerCli
to the project loader